[Image] Implement 'center' and 'repeat' resizeModes #2581

marlenecota · 2019-06-06T19:51:46Z

#2111

ImageViewManager:

Changed Image to ReactImage/Canvas
Moved all Image related files to their own folder.

ReactImage:

Canvas wrapper to play nice with ReactImageBrush
Overrides ArrangeOverride to provide ReactImageBrush the available size (needed for 'center' and 'repeat'

ReactImageBrush:

XamlCompositionBrushBase that switches source brush based on resizeMode
Uses CompositionEffectBrush for 'repeat' resizeMode

BorderEffect:

Header-only implementation of Win2D-like BorderEffect (used in Windows::UI::Composition APIs)
Includes EffectBase as we will probably need to add more Effects in the future (e.g. Blur)

Microsoft Reviewers: Open in CodeFlow

vnext/ReactUWP/Views/cppwinrt/react.uwp.image.ReactImageBrush.g.h

vnext/ReactUWP/Views/Image/ImageViewManager.cpp

vnext/ReactUWP/Views/Image/ReactImage.cpp

ahimberg

🕐

vnext/ReactUWP/Views/Image/ReactImageBrush.h

kmelmon · 2019-06-08T00:28:09Z

vnext/ReactUWP/Views/Image/ReactImageBrush.cpp

+    {
+      bool effectToSurfaceBrushSwitch{
+        ResizeMode() != ResizeMode::Repeat &&
+        !CompositionBrush().try_as<winrt::CompositionSurfaceBrush>() };


!CompositionBrush().try_aswinrt::CompositionSurfaceBrush() [](start = 8, length = 60)

This check feels a bit awkward - we're looking to see if we have an effect brush set, yes? In this case can you check for a non-null m_effectBrush instead? Alternatively, consider refactoring this into a "HasEffectBrush()" helper function #Closed

I'm caching the effect brush in case they have some kind of mechanism to switch an image's resizeMode at runtime, so we can't just check to see if it's not null. What I want to know is if the current composition brush is not a CompositionSurfaceBrush (because that's when we would want to switch brushes). I've added a "UsingSurfaceBrush()" helper function. #Closed

kmelmon · 2019-06-08T00:29:25Z

vnext/ReactUWP/Views/Image/ReactImageBrush.cpp

+        ResizeMode() != ResizeMode::Repeat &&
+        !CompositionBrush().try_as<winrt::CompositionSurfaceBrush>() };
+
+      bool surfaceToEffectBrushSwitch{ ResizeMode() == ResizeMode::Repeat };


bool surfaceToEffectBrushSwitch{ ResizeMode() == ResizeMode::Repeat }; [](start = 6, length = 70)

This seems to be missing a bit of logic (I think it should also check for a null m_effectBrush). #Closed

The effect brush is created and/or retrieved just before the call to this method and the GetOrCreateEffectBrush call is also gated by the same logic (ResizeMode == Repeat).

All other modes use the SurfaceBrush, so we always have to switch brushes if ResizeMode has been set to Repeat. #Resolved

Thanks for the explanation. Reading through this a bit more carefully, I see where I got confused - this function isn't really checking to see if we need to switch brushes as it might already be set to the appropriate brush. It looks like you could remove ShouldSwitchCompositionBrush() altogether, and set the CompositionBrush to what you've already got in compositionBrush. As it stands the code is somewhat confusing.

In reply to: 293640055 [](ancestors = 293640055)

It does check if we need to switch brushes. It's basically just preventing an unnecessary CompositionBrush set when we are switching between two resizeModes that use a CompositionSurfaceBrush (center, contain, cover, stretch). In those cases all we need to do is update CompositionSurfaceBrush.Stretch which is what we do right after we get the SurfaceBrush.

I think the most human way of expressing this is in the negative (i.e. when do we NOT want to switch brushes), so maybe this is more readable?

bool ReactImageBrush::ShouldSwitchCompositionBrush()
{
// The only scenario where we do not want to switch brushes is if:
// 1. We're not switching to the EffectBrush (Repeat is the only resizeMode that uses the EffectBrush) and
// 2. We're already using the SurfaceBrush
return !(ResizeMode() != ResizeMode::Repeat && UsingSurfaceBrush())
}

In reply to: 294010205 [](ancestors = 294010205,293640055)

Maybe this example run-through will help illustrate the point I'm trying to make:

App sets ResizeMode to Repeat => ShouldSwitchCompositionBrush returns true, because ResizeMode() == Repeat.

A little bit later, the size of the element changes and we set the AvailableSize. => ShouldSwitchCompositionBrush again returns true, even though we already have the effect brush set.
I would have expected the function to only return true if we actually need to set a new brush into CompositionBrush().

If I'm reading the code correctly, everything that leads up to the "should switch brushes" check is computing the brush that we should set. Assuming I've got this correct, why can't you just query the current CompositionBrush() and compare to the one you're about to set, and only set if they are different? #Resolved

vnext/ReactUWP/Views/Image/ReactImageBrush.cpp

…eact-native-windows into image-bordereffect

kmelmon

ahimberg · 2019-06-16T17:48:02Z

vnext/ReactUWP/Views/Image/ImageViewManager.cpp

+        ImageSource source{ reactImage->Source() };
+
+        EmitImageEvent(m_wkReactInstance.lock(), reactImage.as<winrt::Canvas>(), succeeded ? "topLoad" : "topError", source);
+        EmitImageEvent(m_wkReactInstance.lock(), reactImage.as<winrt::Canvas>(), "topLoadEnd", source);


as mentioned over chat, we want to change these to fire only when subscribed to, which will require having a shadownode to store if onLoad/etc have been set. #Resolved

oh i thought maybe these were firing more than before your change (for non-http) but that doesn't appear to be the case, future fix...

In reply to: 294093989 [](ancestors = 294093989)

ahimberg

chrisglein · 2019-06-17T17:40:27Z

vnext/ReactUWP/Views/Image/BorderEffect.h

+        };
+
+        HRESULT GetNamedPropertyMappingImpl(
+            _In_count_(mappingCount) const NamedProperty* namedProperties,


Should that be _In_count(namedPropertyCount)? #Resolved

chrisglein · 2019-06-17T17:44:27Z

vnext/ReactUWP/Views/Image/BorderEffect.h

+#include <winrt/Windows.Foundation.h>
+#include <winrt/Windows.UI.h>
+
+#define CATCH_RETURN \


Seems like this (and probably the DECLARE macros below?) should be in a separate header. #Resolved

chrisglein · 2019-06-17T17:54:11Z

vnext/ReactUWP/Views/Image/ImageViewManager.cpp

-    return image;
+    auto reactImage{ ReactImage::Create() };
+
+    reactImage->OnLoadEnd([=](const auto&, const bool& succeeded)


There are a lot of good reasons to avoid default lambda captures entirely. Consider making this an explicit capture of [this, reactImage] (tag is unused?)
Check out F.54, for example. #Resolved

chrisglein · 2019-06-17T18:23:57Z

vnext/ReactUWP/Views/Image/ReactImage.cpp

+        }
+      }
+      catch (winrt::hresult_error const&)
+      {


Should this fire the load end event?
(like above when the memory stream fails?) #Resolved

stecrain

Looking forward to trying out these changes
🚀

chrisglein · 2019-06-17T23:50:06Z

vnext/ReactUWP/Views/Image/ReactImageBrush.cpp

+        }
+
+        // Only switch CompositionBrush if we are switching to/from ResizeMode::Repeat
+        if (ShouldSwitchCompositionBrush())


Should this check be made before creating surfaceBrush and compositionBrush? #Resolved

It was a bad name. The code above determines the correct brush we need to use. This should just check to see if it is different than what we're currently using and set it if that's the case.

In reply to: 294561221 [](ancestors = 294561221)

marlenecota added 6 commits June 3, 2019 19:38

Move ImageViewManager to its own folder

bdf4bcf

Add BorderEffect impl

55f101d

Switch from Image to Canvas

7869db2

Add ReactImageBrush (name TBD)

ccf5a68

wip: couple of fixes

af2e505

Add ReactImage canvas wrapper

634087e

marlenecota requested review from ahimberg and StephenLPeters June 6, 2019 19:51

marlenecota requested a review from a team as a code owner June 6, 2019 19:51

ghost added the vnext label Jun 6, 2019

marlenecota added this to the vNext Milestone 2 milestone Jun 6, 2019

marlenecota requested review from acoates-ms and kmelmon June 6, 2019 21:01

ahimberg reviewed Jun 7, 2019

View reviewed changes

vnext/ReactUWP/Views/cppwinrt/react.uwp.image.ReactImageBrush.g.h Outdated Show resolved Hide resolved

ahimberg reviewed Jun 7, 2019

View reviewed changes

vnext/ReactUWP/Views/Image/ImageViewManager.cpp Outdated Show resolved Hide resolved

ahimberg reviewed Jun 7, 2019

View reviewed changes

vnext/ReactUWP/Views/Image/ImageViewManager.cpp Outdated Show resolved Hide resolved

ahimberg reviewed Jun 7, 2019

View reviewed changes

vnext/ReactUWP/Views/Image/ReactImage.cpp Show resolved Hide resolved

ahimberg suggested changes Jun 7, 2019

View reviewed changes

ghost added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Jun 7, 2019

kmelmon reviewed Jun 7, 2019

View reviewed changes

vnext/ReactUWP/Views/Image/ReactImageBrush.h Show resolved Hide resolved

kmelmon reviewed Jun 8, 2019

View reviewed changes

vnext/ReactUWP/Views/Image/ReactImageBrush.cpp Show resolved Hide resolved

ghost removed the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Jun 14, 2019

marlenecota added 5 commits June 13, 2019 19:23

PR feeedback + add OnLoadEnd event to ReactImage

071564d

Merge branch 'master' into image-bordereffect

6c431d2

Removing accidentally added files

23e7aba

Merge branch 'image-bordereffect' of https://github.com/marlenecota/r…

17630cf

…eact-native-windows into image-bordereffect

Fix break after merge

6727269

kmelmon approved these changes Jun 14, 2019

View reviewed changes

ahimberg reviewed Jun 16, 2019

View reviewed changes

ahimberg approved these changes Jun 16, 2019

View reviewed changes

chrisglein reviewed Jun 17, 2019

View reviewed changes

stecrain approved these changes Jun 17, 2019

View reviewed changes

chrisglein reviewed Jun 17, 2019

View reviewed changes

PR feedback + Moved Image module to separate file

ce9e014

marlenecota merged commit f750896 into microsoft:master Jun 18, 2019

marlenecota mentioned this pull request Jun 18, 2019

[Image] Implement 'repeat' and 'center' resizeMode #2637

Closed

marlenecota deleted the image-bordereffect branch September 8, 2020 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Image] Implement 'center' and 'repeat' resizeModes #2581

[Image] Implement 'center' and 'repeat' resizeModes #2581

marlenecota commented Jun 6, 2019 •

edited

Loading

ahimberg left a comment

kmelmon Jun 8, 2019 •

edited

Loading

marlenecota Jun 14, 2019 •

edited by kmelmon

Loading

kmelmon Jun 8, 2019 •

edited

Loading

marlenecota Jun 14, 2019 •

edited

Loading

kmelmon Jun 14, 2019 •

edited

Loading

marlenecota Jun 15, 2019

kmelmon Jun 15, 2019 •

edited by marlenecota

Loading

kmelmon left a comment

ahimberg Jun 16, 2019 •

edited by marlenecota

Loading

ahimberg Jun 16, 2019

ahimberg left a comment

chrisglein Jun 17, 2019 •

edited by marlenecota

Loading

chrisglein Jun 17, 2019 •

edited by marlenecota

Loading

chrisglein Jun 17, 2019 •

edited by marlenecota

Loading

chrisglein Jun 17, 2019 •

edited by marlenecota

Loading

stecrain left a comment

chrisglein Jun 17, 2019 •

edited by marlenecota

Loading

marlenecota Jun 18, 2019

[Image] Implement 'center' and 'repeat' resizeModes #2581

[Image] Implement 'center' and 'repeat' resizeModes #2581

Conversation

marlenecota commented Jun 6, 2019 • edited Loading

Microsoft Reviewers: Open in CodeFlow

ahimberg left a comment

Choose a reason for hiding this comment

kmelmon Jun 8, 2019 • edited Loading

Choose a reason for hiding this comment

marlenecota Jun 14, 2019 • edited by kmelmon Loading

Choose a reason for hiding this comment

kmelmon Jun 8, 2019 • edited Loading

Choose a reason for hiding this comment

marlenecota Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

kmelmon Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

marlenecota Jun 15, 2019

Choose a reason for hiding this comment

kmelmon Jun 15, 2019 • edited by marlenecota Loading

Choose a reason for hiding this comment

kmelmon left a comment

Choose a reason for hiding this comment

ahimberg Jun 16, 2019 • edited by marlenecota Loading

Choose a reason for hiding this comment

ahimberg Jun 16, 2019

Choose a reason for hiding this comment

ahimberg left a comment

Choose a reason for hiding this comment

chrisglein Jun 17, 2019 • edited by marlenecota Loading

Choose a reason for hiding this comment

chrisglein Jun 17, 2019 • edited by marlenecota Loading

Choose a reason for hiding this comment

chrisglein Jun 17, 2019 • edited by marlenecota Loading

Choose a reason for hiding this comment

chrisglein Jun 17, 2019 • edited by marlenecota Loading

Choose a reason for hiding this comment

stecrain left a comment

Choose a reason for hiding this comment

chrisglein Jun 17, 2019 • edited by marlenecota Loading

Choose a reason for hiding this comment

marlenecota Jun 18, 2019

Choose a reason for hiding this comment

marlenecota commented Jun 6, 2019 •

edited

Loading

kmelmon Jun 8, 2019 •

edited

Loading

marlenecota Jun 14, 2019 •

edited by kmelmon

Loading

kmelmon Jun 8, 2019 •

edited

Loading

marlenecota Jun 14, 2019 •

edited

Loading

kmelmon Jun 14, 2019 •

edited

Loading

kmelmon Jun 15, 2019 •

edited by marlenecota

Loading

ahimberg Jun 16, 2019 •

edited by marlenecota

Loading

chrisglein Jun 17, 2019 •

edited by marlenecota

Loading

chrisglein Jun 17, 2019 •

edited by marlenecota

Loading

chrisglein Jun 17, 2019 •

edited by marlenecota

Loading

chrisglein Jun 17, 2019 •

edited by marlenecota

Loading

chrisglein Jun 17, 2019 •

edited by marlenecota

Loading